-
Notifications
You must be signed in to change notification settings - Fork 345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BannerCallout: VR Implementation Changes #3804
BannerCallout: VR Implementation Changes #3804
Conversation
✅ Deploy Preview for gestalt ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments
color={backgroundColor} | ||
dangerouslySetInlineStyle={{ __style: largePadding }} | ||
display="none" | ||
lgDisplay="block" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only thing that's changing? This seems like a lot of dupe
Can we move into a function or something called getResponsiveSize(size:'sm|'lg|'md')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but also this approach may have React do 3X the work because the components would still be loaded into the virtual dom I think - since it's hidden by CSS. I think ideally this could be 3 separate smaller components, or abstracted at a different level
BannerCallout: VR Implementation Changes
FIGMA